swap arguments in strictEqual() and comment the third argument (test-timers) #21660
Conversation
| @@ -31,7 +31,7 @@ let interval_count = 0; | |||
| clearTimeout(null); | |||
| clearInterval(null); | |||
|
|
|||
| assert.strictEqual(true, setTimeout instanceof Function); | |||
| assert.strictEqual(setTimeout instanceof Function, true); | |||
There was a problem hiding this comment.
Why not assert.ok? And in some other places.
/cc @Trott — do we have a reason for a strict check here?
There was a problem hiding this comment.
assert.ok() would be fine here of course, but I'm not terribly bothered by the strict check either.
There are 5 other files in our test suite that use strictEqual() for an instanceof check. If there is consensus that changing it to assert.ok() would be better, I'd rather change them all at once in a separate PR and add a lint rule.
There was a problem hiding this comment.
Yeah, that should not block this PR, just asking. 😃
test/pummel/test-timers.js
Outdated
| assert.strictEqual(interval_count, 3); | ||
| assert.strictEqual(count4, 11); | ||
|
|
||
| // clearTimeout cleared too many timeouts |
There was a problem hiding this comment.
Nit: Now that I'm looking at it, perhaps the comment might better as:
// Check that the correct number of timers ran.
Trott
added
the
author ready
|
LinuxONE CI failure is almost certainly unrelated. Resume Build isn't working right now on CI, but here's a Rebuild of just that job: https://ci.nodejs.org/job/node-test-commit-linuxone/2762/ |
|
Landed in b70db33 Congratulations @sohailrajdev97 on your first commit in Node.js core! 🎉🎉🎉 |
This commit also comments the third argument PR-URL: #21660 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This commit also comments the third argument PR-URL: #21660 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This addresses a good first issue assigned to me by @Trott . The arguments to all the assert.strictEqual() calls have been swapped and the third argument has been removed and put up as a comment.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes